Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: properly close scheduling timer to avoid checkly test hanging #770

Merged
merged 3 commits into from
Jun 23, 2023

Conversation

clample
Copy link
Collaborator

@clample clample commented Jun 22, 2023

I hereby confirm that I followed the code guidelines found at engineering guidelines

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

With the current canary release from main, test runs with checkly test always takes 20 seconds. This occurs even when the checks are shown as finished after only several seconds. This can be reproduced with time checkly test on the boilerplate example.

This issue is because we aren't closing the timeout for MAX_SCHEDULING_DELAY_EXCEEDED, so the node process hangs until after it runs (in 20 seconds). This issue doesn't affect any public releases since it was merged after v4.0.8.

This PR fixes the issue by closing the timeout after all of the check runs have been scheduled. The code is similar to what we already have for AbstractCheckRunner.allChecksFinished.

Testing:

Tested time checkly test on the boilerplate example. It finishes after ~7 seconds rather than 20.

Also tested that the scheduling delay message is working. To test, I used the following code snippet with checkly test --location eu-south-1:

const checkTime = 30000
const sleepScript = `await new Promise(resolve => setTimeout(resolve, ${checkTime}))`

for (let i = 0; i < 10; i++) {
  new BrowserCheck(`check-${i}`, {
    name: `Check ${i}`,
    code: {
      content: sleepScript,
    }
  })
}

@clample clample requested a review from umutuzgur June 22, 2023 14:38
export default abstract class AbstractCheckRunner extends EventEmitter {
checks: Map<CheckRunId, { check: any, testResultId?: string }>
testSessionId?: string
// If there's an error in the backend and no check result is sent, the check run could block indefinitely.
// To avoid this case, we set a per-check timeout.
timeouts: Map<CheckRunId, NodeJS.Timeout>
schedulingDelayExceededTimeout?: NodeJS.Timeout
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could put it in timeouts, but I prefer having a separate property

@clample clample requested a review from umutuzgur June 22, 2023 15:26
@clample clample merged commit 837619d into main Jun 23, 2023
@clample clample deleted the chrislample/fix-scheduling-message-timeout branch June 23, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants